Define a exhaustive switch errorprone check for conjure sealed unions#3344
Define a exhaustive switch errorprone check for conjure sealed unions#3344
Conversation
Generate changelog in
|
| // Check if it has a nested interface called "Known" | ||
| // Empty conjure unions won't have a Known interface, but they won't be used in switches either by definition | ||
| return ASTHelpers.getEnclosedElements(type.asElement()).stream() | ||
| .anyMatch(element -> element.getKind() == ElementKind.INTERFACE | ||
| && element.getModifiers().contains(Modifier.SEALED) | ||
| && "Known".equals(element.getSimpleName().toString())); |
There was a problem hiding this comment.
Can we instead check whether the type is annotated with @Generated("com.palantir.conjure.java.types.UnionGenerator")?
There was a problem hiding this comment.
Since the generated annotation has "source" retention, wouldn't this fail to match on sealed conjure unions that are published by other services and consumed in the repo running errorprone? It should work for local conjure unions since errorprone runs alongside java compilation.
| case Circle circle -> System.out.println("Circle"); | ||
| default -> System.out.println("Unknown"); |
There was a problem hiding this comment.
| case Circle circle -> System.out.println("Circle"); | |
| default -> System.out.println("Unknown"); | |
| case Circle circle -> System.out.println("Circle"); | |
| case Square square -> System.out.println("Square"); | |
| case Unknown unknown -> System.out.println("Unknown"); | |
| default -> System.out.println("Unknown"); |
Is there a reason not to add all the test cases here? (if we're just testing the behavior with regards to the presence of default, this seems more likely to be the test we want - we can also separately test the check behaves correctly in the absence of some statement, should we want to)
There was a problem hiding this comment.
I believe there's already an existing check that checks for an "unnecessary" default clause if you've written an exhaustive switch statement over an enum/sealed type (not applied in this test, but also not exactly the goal of this check). In this example the default case is necessary since we don't go over all the possible subvariants, but still gets flagged by the check.
|
This feels somewhat misguided, for reasons similar to those mentioned in #3353 (comment). It's pretty common to want to handle one case of a union type and return or throw otherwise. This would make those cases much more verbose. This also means that adding a union variant is going to be a compile break for every consumer of an API which seems undesirable. |
|
@pkoenig10 - I brought this up with the team and we agree that there are valid cases where a developer would want to handle a single variant of a union, and where using a default clause makes sense. We believe exhaustive switch statements are valuable from a safety perspective to make consumers aware when new variants are added to a union via a compile break, especially when the conjure objects are published by another repository. This ensures that they are forced to consciously evaluate the new variant and explicitly decide how to handle it, rather than having it silently fall through to a default clause which may or may not be appropriate. Here's a simple but illustrative example of this that @mpritham found: https://courses.cs.cornell.edu/cs3110/2021sp/textbook/data/catch_all_cases.html Obviously this creates tension with the pattern that you described - I'm not really sure what the best balance is between code safety and developer experience. We thought developers could opt-into using default clauses by just suppressing the errorprone warning, but if this is quite common I imagine that would be frustrating. Definitely open to feedback if this behavior is undesirable, but it is at least consistent with how older union types worked, given adding new variants broke existing visitor code. |
I don't think we should be introducing checks where users are required to suppress the check for some common cases. This teaches users that suppressing warnings is a normal thing to do, increasing the likelihood that they suppress warnings for more important checks. |
|
Fair point, we could be less prescriptive here and instead just warn devs of the potential unexpected consequences of this. We're going to pivot this check to just flagging cases where the switch is already exhaustive and a default case is still included, since that would be logically unnecessary and still would be susceptible to the safety pitfalls of having a default clause. |
DO NOT MERGE (YET)We're currently moving all the error-prone infrastructure in gradle-baseline (including baseline-error-prone, baseline-nullaway) to https://github.com/palantir/baseline-error-prone. This PR should be opened in the new repository once setup is completed. |
|
Hey there, our error-prone infrastructure has been moved to https://github.com/palantir/baseline-error-prone. |
4 participants
Before this PR
If a switch statement over a conjure sealed union type includes a default clause, the Java compiler will
not break when new union variants are added. This is undesirable as it doesn't force code owners to
explicitly acknowledge and handle new types. By using exhaustive switches without default clauses,
the compiler ensures all sealed union consumers must consciously decide how to handle new variants
before their code compiles again.
After this PR
==COMMIT_MSG==
Defines an errorprone check to encourage use of exhaustive switches for conjure sealed union types.
==COMMIT_MSG==
Possible downsides?